Swift 6: complete concurrency checking for StreamChatUI #3660
Conversation
# Conflicts: # Tests/StreamChatUITests/SnapshotTests/ChatChannelList/Search/ChatMessageSearchVC_Tests.swift # Tests/StreamChatUITests/SnapshotTests/ChatMessageList/ChatMessage/ChatMessageMarkdown_Tests.swift
laevandus
added
🎨 SDK: StreamChatUI
Generated by 🚫 Danger |
SDK Size
|
Sources/StreamChatUI/Appearance+Formatters/ChannelNameFormatter.swift
Outdated
Show resolved
Hide resolved
SDK Performance
|
| static var `default`: Appearance = .init() | ||
| static var `default`: Appearance { | ||
| get { queue.sync { _default } } | ||
| set { queue.sync { _default = newValue } } | ||
| } | ||
|
|
||
| private static let queue = DispatchQueue(label: "io.getstream.appearance", target: .global()) | ||
| nonisolated(unsafe) private static var _default: Appearance = .init() |
There was a problem hiding this comment.
On the second look it actually does not feel like a good idea. Since this is used for UI code then it sounds like making the static property MainActor makes more sense here. Otherwise we do unnecessary context switches. Making it main actor will have consequences:
- app level setup code must be done on the main thread
- other code interacting with default must require main thread
There was a problem hiding this comment.
In general, options are:
a) opt-out with nonisolated(unsafe) which kind of loses the point of strict concurrency checking. SDK users can trigger crashes.
b) Internally uses main thread when accessing the default (which should be the 99% of cases when calling it)
static var `default`: Appearance {
get {
MainActor.ensureIsolated { _default }
}
set {
MainActor.ensureIsolated { _default = newValue }
}
}and making Appearance @unchecked Sendable (lot of work to make it fully Sendable since this type captures so many other types, e.g formatters)
c) Make the default @MainActor and add MainActor requirement to all the other callsites
e) Use queue/lock for accessing the default state although it should (almost) always be main thread
There was a problem hiding this comment.
a) loses the point of concurrency checking
b) makes it easier to use default from places without main actor annotation (easier adoption), on the other hand, it does not look the nicest, but at least does not cause performance issues due to context switches
c) cleanest but harder to use for users since it forces MainActor
e) performance wise not good because this is used by UI code and should be almost always run on the main thread (that said, SDK users could have setup running on a background thread)
Sources/StreamChatUI/ChatMessageList/Attachments/Poll/PollsConfig.swift
Outdated
Show resolved
Hide resolved
...ces/StreamChatUI/ChatMessageList/ChatMessage/Decorators/ChatMessageHeaderDecoratorView.swift
Outdated
Show resolved
Hide resolved
| ) { | ||
| self.listView = listView | ||
| self.impactFeedbackGenerator = impactFeedbackGenerator | ||
| self.impactFeedbackGenerator = impactFeedbackGenerator ?? UIImpactFeedbackGenerator(style: .medium) |
There was a problem hiding this comment.
Xcode 15 had compiler crashes and this was the only way I could get it compiling
SDK Size
|
| static var `default`: Appearance = .init() | ||
| static var `default`: Appearance { | ||
| get { | ||
| MainActor.ensureIsolated { _default } | ||
| } | ||
| set { | ||
| MainActor.ensureIsolated { _default = newValue } | ||
| } | ||
| } | ||
|
|
||
| // Shared instance is mutated only on the main thread without explicit | ||
| // main actor annotation for easier SDK setup. | ||
| nonisolated(unsafe) private static var _default: Appearance = .init() |
There was a problem hiding this comment.
At first I was thinking about making it @MainActor, but this is used in multiple init methods meaning all of these must be @MainActor as well. Moreover, it might make the SDK initialization more complex as well since SDK users need to make sure Appearance is changed only from the MainActor.
a) we just make default nonisolated(unsafe)
b) we do what we have above which internally forcing main actor (for making sure SDK users do not corrupt the default by changing it from background threads)
SDK Performance
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Important
Merges to the main
swift-6branch, notdevelop🔗 Issue Links
Resolves IOS-735
🎯 Goal
📝 Summary
@Sendablein most of the casesStreamConcurrency.onMain)nonisolatedbecause any thread thread could call it and often MainActor guarded type implements these delegates (e.g. UIViewController subclass)nonisolated(unsafe)a lot because otherwise most of the test must be rewritten (huge effort)@MainActorrequirement🛠 Implementation
LLC controllers support any queue for delegate callbacks and completion handlers. Therefore we need to ensure that these run on the main thread when updating UI code. This is where the custom
MainActor.ensureIsolated(_)comes into the play. It just ensures that code triggered from these code paths are always on the main thread.Public API which requires MainActor are annotated with
@preconcurrency @MainActorwhich is the backwards compatibility support. In the next major version we would clean it up.🧪 Manual Testing Notes
Manual testing in #3661
☑️ Contributor Checklist
docs-contentrepo